bpo-30101: Add support for curses.A_ITALIC.#1015
Conversation
|
Hello, and thanks for your contribution! I'm a bot set up to make sure that the project can legally accept your contribution by verifying you have signed the PSF contributor agreement (CLA). Unfortunately we couldn't find an account corresponding to your GitHub username on bugs.python.org (b.p.o) to verify you have signed the CLA. This is necessary for legal reasons before we can look at your contribution. Please follow the steps outlined in the CPython devguide to rectify this issue. Thanks again to your contribution and we look forward to looking at it! |
berkerpeksag
left a comment
berkerpeksag
left a comment
There was a problem hiding this comment.
Thanks for the PR, but we need two more things to move this forward:
- Create an issue on bugs.python.org and update the PR title to include the issue number
- Document
A_ITALICinDoc/library/curses.rst(additionally with a.. versionadded:: 3.7markup)
There was a problem hiding this comment.
A_ITALIC is rather a new addition so I think we should conditionally set it:
#ifdef A_ITALIC
SetDictInt("A_ITALIC", A_ITALIC);
#endif
|
What should I prefix with |
|
Good question. I'd add the following after the first table at https://docs.python.org/3/library/curses.html#constants: .. versionadded:: 3.7
``A_ITALIC`` was added.Of course, |
Eijebong
changed the title
There was a problem hiding this comment.
A_ITALIC is a ncurses extension, not X/Open standard. So I think it should be put after A_VERTICAL and prefixed with a line /* ncurses extension */.
There was a problem hiding this comment.
Please don't start to "document" constants here, it's not the right place.
There was a problem hiding this comment.
I am not sure this doc change is suitable. This list lacks many options and seems only list some strict sysv options here.
There was a problem hiding this comment.
Where should I document this then ?
There was a problem hiding this comment.
Honestly speaking I prefer to leave this out (no change) in this issue and just change the code. The documentation should belong to another issue, whether list all options and document their limitations, or clarifying these are some commonly available ones.
There was a problem hiding this comment.
@zhangyangyu can you please stop mixing things up? I don't even know what other options are you talking about. This PR is about exposing the A_ITALIC constant. Please open a new issue to discuss your ideas on curses documentation on bugs.python.org.
The Python stdlib is full of things that are only available in specific platforms and that doesn't stop us from documenting them. See the os, sys, select modules for example. If you want to add a note that says "some of these constants may not be available in your choice of curses implementation", that's fine and we can add it in this PR.
There was a problem hiding this comment.
@berkerpeksag if you don't know what other options you should first read them. I have said in my comment the documentation problem needs another issue, not in this one.
Adding the note as you said is not suitable. Why do you only document A_ITALIC not other options? e.g. A_LEFT, A_LOW. A_ITALIC should be solved with them but not singly added which IMHO leads to confusion.
So of course we need to document A_ITALIC. But instead of singly added it in this issue, We could either opening an issue solving the documentation problem first, make this issue dependent on it, or only changing code in this issue and solve the documentation in another issue once. Considering our pace I prefer the latter choice.
There was a problem hiding this comment.
I don't really want to keep this simple PR open for months or merge it without documentation changes because you've invented a new problem. I don't see any previous documentation issues relevant to this hypothetical problem on bugs.p.o. Use bugs.p.o for your random ramblings, and keep code review comments specific to the issue.
There was a problem hiding this comment.
@zhangyangyu: "I am not sure this doc change is suitable. This list lacks many options and seems only list some strict sysv options here."
I concur with @berkerpeksag: while your comment is correct, we should not block this change, please open a new issue to request to complete/update the documentation.
vstinner
dismissed
berkerpeksag’s stale review
@berkerpeksag: "Thanks for the PR, but we need two more things to move this forward: (...)" => done, I update the status of the PR
There was a problem hiding this comment.
Style nit: Please use three spaces:
.. versionadded:: 3.7
``A_ITALIC`` was added.
We now only need a |
|
Sure, I'll do it ^^ Edit: Done |
Eijebong
force-pushed
the
master
branch
2 times, most recently
from
8f4ce7a to
f40ac69
Compare
|
@Eijebong ，if your name is not in MISC/ACKs, also need to add it. |
|
This appears to not be working on the my build (yappt-PAYrWqqE) ✘-1 [brad@bradmac:~/Code/yappt] [master ↑·1|✚ 2…4] $ python
Python 3.7.0 (default, Jul 23 2018, 20:22:55)
[Clang 9.1.0 (clang-902.0.39.2)] on darwin
Type "help", "copyright", "credits" or "license" for more information.
>>> import curses
>>> curses.A_ITALIC
Traceback (most recent call last):
File "<stdin>", line 1, in <module>
AttributeError: module 'curses' has no attribute 'A_ITALIC'
>>> |
|
@bradwood The option is not always available. It depends on your underlying curses implementation library. For example, ncurses add the option in version 6.0, see https://invisible-island.net/ncurses/announce-6.0.html, so on my environment with a ncurses 5.0, it's not available. |
|
Ok.. Having now upgraded the curses on my Mac to 6.1, how do I make python see the latest curses library? Do I need to rebuild python pointing to those headers, or can I just set an environment variable? like LDFLAGS or some such? |
|
@bradwood I think you need recompile. |
6 participants
No description provided.